-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[RyuJIT Wasm] Cast Operations Follow Up #122862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <[email protected]>
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
…some more casting cases; Add genFloatToIntCast
521f043 to
c88d66b
Compare
…nt saturating cast opcodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR extends WebAssembly JIT cast operation support by implementing saturating float-to-int conversions and small integer casts with proper sign extension.
Changes:
- Added support for casts from small int types (byte, short) with proper sign/zero extension
- Implemented float-to-int casts using saturating non-trapping conversion instructions (requiring 2-byte opcodes)
- Extended opcode type from uint8_t to uint16_t to support 2-byte instruction encoding
- Moved GenIntCastDesc constructor out of TARGET_64BIT ifdefs to support WASM
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/morph.cpp | Skip helper call morphing for float-to-long casts on WASM (direct saturating instruction support) |
| src/coreclr/jit/instrswasm.h | Add load-with-extension, conversion, and saturating truncation instructions; align formatting |
| src/coreclr/jit/gentree.cpp | Update cost estimation for WASM cast operations |
| src/coreclr/jit/emitwasm.cpp | Change opcode type to uint16_t and add 2-byte opcode emission logic |
| src/coreclr/jit/cpp.hint | Add multiple assert macro definitions for IntelliSense |
| src/coreclr/jit/codegenwasm.cpp | Implement genIntToIntCast, genFloatToIntCast, genFloatToFloatCast; add PackTypes helper; handle TYP_INT from TYP_LONG in genCodeForLclVar |
| src/coreclr/jit/codegenlinear.cpp | Move genCodeForCast and GenIntCastDesc to support WASM; add WASM to TARGET_64BIT conditionals |
| src/coreclr/jit/codegeninterface.h | Change instInfo type to uint16_t for WASM |
| src/coreclr/jit/codegen.h | Add WASM to TARGET_64BIT conditionals in GenIntCastDesc |
d86e911 to
cc76584
Compare
|
@dotnet/jit-contrib Initial feedback should be implemented now so this is ready for another review pass! |
| // | ||
| static constexpr uint32_t PackOperAndType(genTreeOps oper, var_types toType, var_types fromType) | ||
| { | ||
| if (fromType == TYP_BYREF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also convert TYP_REF
| // | ||
| static constexpr uint32_t PackTypes(var_types toType, var_types fromType) | ||
| { | ||
| if (toType == TYP_BYREF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| m_extendSrcSize = srcSize; | ||
| } | ||
|
|
||
| #ifndef TARGET_WASM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this ifdef? I think things will work correctly without it (since we don't actually do any containment for casts yet).
| GetEmitter()->emitIns(INS_i32_and); | ||
| ins = (toType == TYP_LONG) ? INS_i64_extend_u_i32 : INS_none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| GetEmitter()->emitIns(INS_i32_and); | |
| ins = (toType == TYP_LONG) ? INS_i64_extend_u_i32 : INS_none; | |
| ins = INS_i32_and; |
This type of cast always produces a TYP_INT value.
| // Return Value: | ||
| // oper and the types packed into an integer that can be used as a switch value/case | ||
| // | ||
| static constexpr uint32_t PackOperAndType(genTreeOps oper, var_types toType, var_types fromType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variant looks unused.
| instruction ins = INS_none; | ||
| assert(fromType == TYP_INT || fromType == TYP_LONG); | ||
|
|
||
| genConsumeOperands(cast); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need an NYI for checked (gtOverflow) casts. Also applies to genFloatToIntCast.
This PR addresses some review feedback in #122301 and fills in more codegen for casts on Wasm.
Notably:
GenIntCastDescwas moved out of ifdefs and adapted for Wasm to support this. We don't yet support pushing down casts into loads (i.e., theLOAD_*integer cast types).